stream action buttons: Implement search messages in stream #5248

SilentCruzer · 2022-02-18T13:39:22Z

closes: #4675

Changes Made:

Commit: narrow: Add optional param streamId in search narrow
- Added streamId as an optional parameter in the narrow of type search. This will help to modify the previous search by constructing a new narrow. If the streamId is not specified, it will work normally as before. If it is mentioned, then it searches the message in the given stream
Commit: stream action buttons: Implement search messages in stream
- Added a search message button in stream actions.
- Included an optional parameter streamId in navigatetoSeach function.

Video:

search.mp4

gnprice

Thanks @SilentCruzer! Comments below.

In the UI, in this version once you navigate to the screen for searching within the stream, it looks identical to the normal search screen -- there's no visible indication of any kind to make clear that you're doing a search that's specific to one stream.

I feel like there should be such an indication, but I don't have a firm idea of what it should look like. @alya, do you perhaps have a suggestion?

gnprice · 2022-04-22T01:02:54Z

src/utils/narrow.js

+      streamId !== undefined
+        ? [
+            { operator: 'search', operand: query },
+            { operator: 'stream', operand: streamId },


Instead of returning the stream ID here, we should handle it in the same way as in the other two places where we handle operator: 'stream' in this function.

gnprice · 2022-04-22T01:08:54Z

src/utils/narrow.js

+    // Below we are checking if the streamId is mentioned or not
+    // If it is mentioned, it will construct a narrow based on that.


These two lines are an example of a comment that just repeats what the code already straightforwardly says. So it's better to leave them out and let the code speak for itself.

That's good because it means fewer things to read when seeking to understand the code. It also means one fewer thing that can get out of sync when the code changes, and become wrong -- as is, this sort of comment is highly vulnerable to becoming wrong in the future, and a comment that's wrong is of negative value.

gnprice · 2022-04-22T01:28:55Z

src/utils/narrow.js

+    // The search narrow can behave in two ways,
+    // one where there is no filters, so the user can search a message in all the streams
+    // the second where the user wants to search in a particular stream


These three lines are providing potentially helpful context on what we intend for these narrows to mean -- what the meaning is of a search narrow with streamId present or of one with it absent.

The most helpful place for that sort of information to live is on the type definition, instead of down here in the implementation of one of the functions that consumes the narrow. That way, it's in a central place that is natural to refer to whenever someone's trying to understand these objects.

Specifically, let's put it right above the line where you added streamId in the type definition.

Also, it's helpful for documentation like this to be concise. Try and see if you can find a way to express the essential content of what this is saying but in fewer words.

gnprice · 2022-04-22T01:30:52Z

src/search/SearchMessagesScreen.js

@@ -18,7 +18,7 @@ import { fetchMessages } from '../message/fetchActions';
 type OuterProps = $ReadOnly<{|
  // These should be passed from React Navigation
  navigation: AppNavigationProp<'search-messages'>,
-  route: RouteProp<'search-messages', void>,
+  route: RouteProp<'search-messages', {| streamId: number |}>,


This type needs to match up with what's passed to it from navActions. See previous discussion: #5189 (comment)

gnprice · 2022-04-22T01:32:47Z

src/action-sheets/index.js

+const searchMessage = ({ streamId }) => {
+  NavigationService.dispatch(navigateToSearch(streamId));
+};
+searchMessage.title = 'Search Message';


Let's make this "Search in stream".

gnprice · 2022-04-22T01:40:09Z

src/action-sheets/index.js

+const searchMessage = ({ streamId }) => {
+  NavigationService.dispatch(navigateToSearch(streamId));
+};
+searchMessage.title = 'Search Message';
+searchMessage.errorMessage = 'Failed to open search';


You'll have a rebase conflict to resolve here, sorry -- see f32a679 for the relevant change.

SilentCruzer · 2022-04-22T15:03:59Z

In the UI, in this version once you navigate to the screen for searching within the stream, it looks identical to the normal search screen -- there's no visible indication of any kind to make clear that you're doing a search that's specific to one stream.

Maybe we can try changing the placeholder text in the search bar. Currently, it simply shows "Search",

Say if we are searching in the mobile-team stream, we can use a UI like this:

We are already passing the streamId in the SearchMessages component, we can get the stream name using that. The screen component already has the searchPlaceholder as a prop, so we can pass the stream name there.

gnprice · 2022-04-22T15:42:23Z

Maybe we can try changing the placeholder text in the search bar.

Yeah, good idea -- that'd certainly be a good start.

Once the user has typed a query, I'd still worry a bit that it looks like they're getting results for simply the query they typed, when in fact they're getting results for that query plus limiting to the stream. So I think ideally there'd be some other indication, one that stays visible after entering a query.

alya · 2022-04-27T06:51:41Z

I think we can show stream:foo in the search box, and put the cursor after it for the user to enter the query. That's the approach that we use in the webapp, and it seems reasonable for mobile as well.

Does that sound right? If we do so, deleting the stream:foo part should do a regular search with no stream restriction.

SilentCruzer · 2022-04-29T18:21:02Z

I think we can show stream:foo in the search box, and put the cursor after it for the user to enter the query. That's the approach that we use in the webapp, and it seems reasonable for mobile as well.

Does that sound right? If we do so, deleting the stream:foo part should do a regular search with no stream restriction.

This seems good to me, For this, we might need to add a feature that allows the regular search bar to recognize the keyword stream: which can be whole another issue, I will try to update this pr with the searching using `stream: feature.

SilentCruzer · 2022-04-30T19:03:54Z

@alya, I have made the suggested changes. Here is the demo of how the search screen looks with new changes:

search.mp4

If the stream: foo is removed, then it behaves like a normal search. But if stream:foo is typed manually in the normal search bar, it does not narrow down the search instead it behaves like the normal one.

I wanted to add the feature where the user can manually type stream:foo and narrow down the search, so I just wanted to be sure of the implementation before I add it. Does it make sense to use the streamAutoComplete component which we use in the chat screen to mention streams for autocompleting the stream names and get the streamId at the same time?

alya · 2022-05-24T22:38:31Z

Cool, thanks! We should definitely make sure that stream:foo works, and behaves the same way whether you typed it manually or not.

@chrisbobbe or @gnprice would be in a better position to comment on the implementation question.

gnprice · 2022-05-25T04:08:14Z

We should definitely make sure that stream:foo works, and behaves the same way whether you typed it manually or not.

@chrisbobbe or @gnprice would be in a better position to comment on the implementation question.

I agree this would be good to have; it's in the tracker as #4903. I think it will be a somewhat bigger project than this PR was: it'll mean we want to start actually parsing the user's input in the search box, as Zulip search syntax.

gnprice reviewed Apr 22, 2022

View reviewed changes

SilentCruzer added 3 commits May 1, 2022 00:15

narrow: Add optional param streamId in search narrow

7821efc

Input: Add optional param prefixText in Input

0b1d8f9

stream action buttons: Implement search messages in stream

3b5dc05

SilentCruzer force-pushed the searchAction branch from 2246e6b to 3b5dc05 Compare April 30, 2022 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream action buttons: Implement search messages in stream #5248

stream action buttons: Implement search messages in stream #5248

SilentCruzer commented Feb 18, 2022

gnprice left a comment

gnprice Apr 22, 2022

gnprice Apr 22, 2022

gnprice Apr 22, 2022

gnprice Apr 22, 2022

gnprice Apr 22, 2022

gnprice Apr 22, 2022

SilentCruzer commented Apr 22, 2022

gnprice commented Apr 22, 2022

alya commented Apr 27, 2022

SilentCruzer commented Apr 29, 2022 •

edited

Loading

SilentCruzer commented Apr 30, 2022

alya commented May 24, 2022

gnprice commented May 25, 2022

		// Below we are checking if the streamId is mentioned or not
		// If it is mentioned, it will construct a narrow based on that.

stream action buttons: Implement search messages in stream #5248

Are you sure you want to change the base?

stream action buttons: Implement search messages in stream #5248

Conversation

SilentCruzer commented Feb 18, 2022

gnprice left a comment

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

gnprice Apr 22, 2022

Choose a reason for hiding this comment

SilentCruzer commented Apr 22, 2022

gnprice commented Apr 22, 2022

alya commented Apr 27, 2022

SilentCruzer commented Apr 29, 2022 • edited Loading

SilentCruzer commented Apr 30, 2022

alya commented May 24, 2022

gnprice commented May 25, 2022

SilentCruzer commented Apr 29, 2022 •

edited

Loading